Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sdjournal: fix input argument type in C.Malloc call #362

Merged
merged 1 commit into from
Mar 25, 2021
Merged

sdjournal: fix input argument type in C.Malloc call #362

merged 1 commit into from
Mar 25, 2021
+1 −1

Conversation

lsm5
Copy link
Contributor

@lsm5 lsm5 commented Mar 24, 2021 •
edited by lucab
Loading

podman v3.1.0-rc2 uses v22 go-systemd and fedora rpm builds were failing
for 32-bit arches (i686 and armv7hl).

This commit conditionally defines id128StringMax based on
32-bit v/s 64-bit arches.

Signed-off-by: Lokesh Mandvekar lsm5@fedoraproject.org
Closes: #363

/cc @vrothberg @ashcrow @mheon @rhatdan

@lsm5 lsm5 mentioned this pull request Mar 24, 2021
Closed
mheon
mheon reviewed Mar 24, 2021
View reviewed changes
sdjournal/journal_64.go Outdated Show resolved Hide resolved
lucab
lucab suggested changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, but a bug report beforehand with logs would have been much appreciated.
I'm very doubtful of this patch, can you please share the actual build failure on whatever 32b platform you have at hand?

From a quick skim, I guess the actual bug is that the cast-target type should have been C.size_t instead.

@lsm5
Copy link
Contributor Author

lsm5 commented Mar 24, 2021

Thanks for the PR, but a bug report beforehand with logs would have been much appreciated.
I'm very doubtful of this patch, can you please share the actual build failure on whatever 32b platform you have at hand?

I'll regenerate the build failure log

@lsm5
Copy link
Contributor Author

lsm5 commented Mar 24, 2021

filed #363

rhatdan
rhatdan reviewed Mar 24, 2021
View reviewed changes
sdjournal/journal_64.go Outdated Show resolved Hide resolved
@lucab
Copy link
Contributor

lucab commented Mar 24, 2021

cannot use id128StringMax (type _Ctype_ulong) as type _Ctype_uint in argument to _Cfunc__CMalloc

Indeed this hints at a type mismatch in C.Malloc argument, which should be a C.size_t. See suggestion for a better patch in the review comment.

@lsm5
Use C.size_t instead of C.ulong
cdfecbb 
This will avoid type mismatch issues for 32-bit arches
which was earlier causing build failures for
podman v3.1.0-rc2.

Thanks to Luca Bruno <lucab@redhat.com> for the review and
patch simplification :)

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
@lsm5
Copy link
Contributor Author

lsm5 commented Mar 24, 2021

cannot use id128StringMax (type _Ctype_ulong) as type _Ctype_uint in argument to _Cfunc__CMalloc

Indeed this hints at a type mismatch in C.Malloc argument, which should be a C.size_t. See suggestion for a better patch in the review comment.

Yes, patch updated. Thanks a lot @lucab !!

@lsm5 lsm5 changed the title Conditionally define id128StringMax Use C.size_t instead of C.ulong Mar 24, 2021
@lucab lucab enabled auto-merge March 25, 2021 08:44
@lucab lucab changed the title Use C.size_t instead of C.ulong sdjournal: fix input argument type in C.Malloc call Mar 25, 2021
@lucab lucab merged commit a99570e into coreos:master Mar 25, 2021
@lsm5 lsm5 deleted the 32-bit-fix branch March 25, 2021 12:29
@lsm5
Copy link
Contributor Author

lsm5 commented Mar 25, 2021

@lucab thanks a lot! I think we'd need a new release with this change /cc @mheon.

@mheon
Copy link

mheon commented Mar 25, 2021

Would be appreciated, though if not possible we can revert to an earlier release.

@lucab
Copy link
Contributor

lucab commented Apr 1, 2021

Tagged: https://github.com/coreos/go-systemd/releases/tag/v22.3.1

@mheon
Copy link

mheon commented Apr 1, 2021

Greatly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhatdan rhatdan rhatdan left review comments

@mheon mheon mheon left review comments

@lucab lucab lucab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

id128StringMax in sdjournal/journal.go fails on 32-bit arches
4 participants
@lsm5 @lucab @mheon @rhatdan